treat string partition ranges as sensitive data by jgao54 · Pull Request #4501 · PeerDB-io/peerdb

jgao54 · 2026-06-26T01:46:31Z

Before introducing parallel snapshotting for arbitrary string watermark column, in order to avoid sensitive data from being accidentally exposed in db/Temporal as plaintext, this PR will offload partition ranges to db (encrypted) before return the GetQRepPartitions Activity result, and then restored in the ReplicateQRepPartitions Activity, to avoid crossing Temporal boundary. Long term, offloading all partition ranges to DB is also ideal, in order to not having to worry about Temporal payload limit.

This PR introduces a new table metadata_qrep_partition_ranges for storing partition ranges. I also evaluated building on top of existing peerdb_stats.qrep_partitions but that table is intended for monitoring so adding operational logic to it is not ideal. This PR also makes sure that sensitive ranges are removed before writing to peerdb_stats.qrep_partitions, which are not being used afaik.

Open to feedback on what we should deem as sensitive that should be encrypted (e.g. all string vs. non-UUIDs vs. only encrypt email-shaped string). Currently all string ranges are encrypted.

Testing: add integration test coverage for offload/restore correctness logic; as well as e2e test for the workflow, and that table is cleanup when mirror is dropped.

Fixes: DBI-869

codecov · 2026-06-26T02:03:27Z

❌ 2 Tests Failed:

Tests completed	Failed	Passed	Skipped
2645	2	2643	336

View the top 2 failed test(s) by shortest run time

github.com/PeerDB-io/peerdb/flow/e2e::TestApiMongo

Stack Traces | 0.01s run time

=== RUN   TestApiMongo
=== PAUSE TestApiMongo
=== CONT  TestApiMongo
--- FAIL: TestApiMongo (0.01s)

github.com/PeerDB-io/peerdb/flow/e2e::TestApiMongo/TestEditTablesBeforeResync

Stack Traces | 0.08s run time

=== RUN   TestApiMongo/TestEditTablesBeforeResync
=== PAUSE TestApiMongo/TestEditTablesBeforeResync
=== CONT  TestApiMongo/TestEditTablesBeforeResync
    api_test.go:2336: 
        	Error Trace:	.../flow/e2e/api_test.go:2336
        	Error:      	Received unexpected error:
        	            	rpc error: code = Internal desc = invalid mirror: rpc error: code = FailedPrecondition desc = failed to validate source connector mongo: oplog retention must be set to >= 24 hours, but got 23.000000
        	Test:       	TestApiMongo/TestEditTablesBeforeResync
    api_test.go:51: begin tearing down postgres schema api_igamtnbk
--- FAIL: TestApiMongo/TestEditTablesBeforeResync (0.08s)

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

github-actions · 2026-06-26T02:06:32Z

❌ Test Failure

Analysis: The e2e test Test_MySQL_String_Partition_Key_Sensitive_Range_Handling fails deterministically with a pgx scan mismatch ("number of field descriptions must equal number of destinations, got 2 and 3") across all 4 matrix variants, indicating a real code bug in this PR rather than a flaky failure.
Confidence: 0.9

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

github-actions · 2026-06-26T03:40:55Z

❌ Test Failure

Analysis: A new MySQL→ClickHouse e2e test added in this PR fails deterministically with a pgx scan column-mismatch ("got 2 and 3") in both suite variants, indicating a real code bug rather than a flaky/timeout/race failure.
Confidence: 0.95

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

github-actions · 2026-06-26T04:13:57Z

❌ Test Failure

Analysis: The PR's own new test Test_MySQL_String_Partition_Key_Sensitive_Range_Handling fails with a deterministic assertion error (clickhouse_mysql_test.go:2096) on the MySQL→ClickHouse path across all four MySQL/MariaDB matrix jobs, indicating a real bug in the PR's string-partition-range handling rather than flakiness.
Confidence: 0.95

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

github-actions · 2026-06-26T04:23:34Z

❌ Test Failure

Analysis: The PR's own newly-added test Test_MySQL_String_Partition_Key_Sensitive_Range_Handling fails deterministically across all four MySQL/MariaDB matrix configs with a consistent "Should NOT be empty" data assertion, indicating a real bug in the PR's sensitive-partition-range feature rather than flakiness.
Confidence: 0.95

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

claude · 2026-06-26T08:09:40Z

Code Review

Bug: partitionsContainSensitiveRanges early return on nil partition

File: flow/connectors/external_metadata/store.go L419-L422

partitionsContainSensitiveRanges returns false immediately when it encounters a partition with nil or nil Range, instead of using continue to check the remaining partitions. If a nil-Range partition appears before a StringRange partition in the slice, the function incorrectly returns false, and OffloadSensitivePartitionRanges skips encryption — allowing sensitive string partition data (potentially PII like email addresses) to flow through Temporal in plaintext.

Fix: Change return false to continue on line 421.

Also checked for CLAUDE.md / REVIEW.md compliance — no issues found.

ilidemi · 2026-06-27T01:01:23Z

+		}
+		insertRows = append(insertRows, []any{parentMirrorName, runUUID, partition.PartitionId, key.ID, encrypted})
+
+		partition.Range = &protos.PartitionRange{


Add an Offloaded: true somewhere? Would be ideal to also fail at consumption points if it's accidentally true but haven't checked how feasible that is.

Alternative: add offloaded_range to the oneof range proto and at least have to handle it in all the switches

ilidemi · 2026-06-27T01:21:19Z

+		if partition == nil || partition.Range == nil {
+			continue
+		}
+		// String ranges may contain PII data like email, so deem it as sensitive.


I'm leaning towards doing this for all types, just because you can extract signal from things and we want to be able to snapshot all sorts of tables. Let's discuss with the team?

discussed with team: conclusion is to encrypt this for all data types, as integer PKs may potentially contain sensitive data as well like government-issued IDs; especially given that we also support secondary indices as partition keys.

ilidemi · 2026-06-27T01:39:20Z

+-- through Temporal. Today this is used only for sensitive partition ranges.
+-- The table name and columns are deliberately generic so a future change can
+-- offload all partition ranges to catalog to stay under Temporal's payload limit.
+CREATE TABLE IF NOT EXISTS metadata_qrep_partition_ranges (


Have you thought about limiting retention? I think something like indexed inserted_at and a scheduled task that wipes everything older than 30 days should be about it. Going too low would risk breaking unfortunate long snapshots or complicating the retention logic, blanket 30 seems like a reasonable default

Inserted_at makes a lot of sense.

jgao54 · 2026-06-30T05:01:33Z

 	jobName string,
 	startTime time.Time,
 ) error {
+	if partition.RangeOffloaded {


this was missed earlier but we also serialize the entire QRepPartition in metadata_qrep_partitions.
so here we sanitize this as well.

github-actions · 2026-06-30T05:16:15Z

❌ Test Failure

Analysis: A deterministic regression in the PR's own QRep partition-range code: Test_Offload_Partition_Ranges fails identically across all three matrix shards with a fixed "expected int(8), got int32(1)" assertion, cascading into the other QRep/API tests.
Confidence: 0.96

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

claude · 2026-06-30T06:47:37Z

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

github-actions · 2026-06-30T07:01:12Z

❌ Test Failure

Analysis: The same partition-range/QRep tests (Test_Offload_Partition_Ranges, TestApiPg/My TestQRep with "should have 2 items but has 3") fail deterministically across all three matrix jobs, directly matching this PR's partition-range changes, indicating a real regression rather than a flaky failure.
Confidence: 0.95

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

claude · 2026-06-30T07:25:54Z

Code review: 1 issue found. Checked for bugs and CLAUDE.md compliance. Bug: QRepHasNewRows does not accept runUUID - offloaded partition ranges never restored. File: flow/workflows/qrep_flow.go:470. runUUID is passed as a third activity argument but the function signature only accepts (ctx, config, last). Temporal silently drops extra args. Since runUUID is never received, RestoreOffloadedPartitionRanges is never called inside QRepHasNewRows. When last.RangeOffloaded==true (Range is nil), the nil check short-circuits and returns maxValue!=nil which is always true for non-empty tables. Fix: Add runUUID string to QRepHasNewRows signature and call RestoreOffloadedPartitionRanges at start. See inline comment for details.

github-actions · 2026-06-30T07:33:27Z

❌ Test Failure

Analysis: All four failing tests are QRep sync operations that time out identically across every matrix job, matching exactly this PR's change to QRep partition-range handling ("treat string partition ranges as sensitive data") — a deterministic regression, not flakiness.
Confidence: 0.9

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

github-actions · 2026-06-30T07:33:28Z

❌ Test Failure

Analysis: A consistent set of QRep tests (including the PR's own Test_Offload_Partition_Ranges) time out identically across all matrix jobs, indicating the "treat partition ranges as sensitive data" change broke the offload/restore path so QRep syncs never complete — a real bug, not a flaky failure.
Confidence: 0.93

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

jgao54 · 2026-06-30T19:01:15Z

addressed review feedback:

always encrypt non-nil ranges regardless of type
added range_offloaded field to indicate whether a partition is offloaded
update schema to include inserted_at timestamp

additional changes:

make sure metadata_qrep_partitions also does not store serialized partition in plaintext
add dynconf PEERDB_OFFLOAD_PARTITION_RANGES (allow disabling offload/encryption for OSS)
update tests accordingly

what this PR doesn't cover:

TTL implementation (there are a number of tables that we'd actually want to implement TTL for; so better to assess them holistically)
support offload/encryption for stand-alone QRepFlowWorkflow (legacy feature)

ilidemi · 2026-07-02T01:19:48Z

+		// Clone rather than mutate in place because this partition is shared with the pull
+		// goroutine, cloning keeps us from relying on implicit ordering to avoid a write race.
+		// TODO: drop the sync_partition column entirely; nothing reads it back.
+		partition = proto.Clone(partition).(*protos.QRepPartition)


Suggested change

partition = proto.Clone(partition).(*protos.QRepPartition)

partition = proto.CloneOf(partition)

- decouple offload and restore (so restore derive based on range_offloaded flag) - always encrypt non-nil ranges regardless of type - update schema to include inserted_at timestamp - make sure metadata_qrep_partitions also does not store serialized partition in plaintext - add dynconf PEERDB_OFFLOAD_PARTITION_RANGES to allow disabling offload/encryption behind a FF - update tests

github-actions · 2026-07-02T03:53:40Z

🔄 Flaky Test Detected

Analysis: Kafka e2e tests timed out at identical durations receiving empty messages (broker delivery/infra issue) and a Temporal continue-as-new count race ("1 not greater than 1") failed in only one matrix variant, both unrelated to this PR's partition-range change — classic flaky signatures.
Confidence: 0.85

✅ Automatically retrying the workflow

View workflow run

github-actions · 2026-07-02T04:10:27Z

🔄 Flaky Test Detected

Analysis: A single-matrix e2e failure where the CI MongoDB container transiently reported oplogMinRetentionHours=23 despite being started with --oplogMinRetentionHours 24, causing a validation error unrelated to the PR's partition-range sensitive-data changes.
Confidence: 0.85

✅ Automatically retrying the workflow

View workflow run

jgao54 mentioned this pull request Jun 26, 2026

mongo: partition snapshots for numeric and string _id collections #4487

Merged

jgao54 force-pushed the offload-restore-sensitive-data branch from 15d2fe1 to f6df0b1 Compare June 26, 2026 03:21

jgao54 force-pushed the offload-restore-sensitive-data branch 2 times, most recently from 9816583 to c926f03 Compare June 26, 2026 03:55

jgao54 force-pushed the offload-restore-sensitive-data branch 2 times, most recently from f3e2322 to 5b2f94d Compare June 26, 2026 07:22

jgao54 marked this pull request as ready for review June 26, 2026 07:57

jgao54 requested a review from a team as a code owner June 26, 2026 07:57

ilidemi reviewed Jun 27, 2026

View reviewed changes

jgao54 force-pushed the offload-restore-sensitive-data branch from b4d9ba9 to 478c3f2 Compare June 30, 2026 04:54

jgao54 commented Jun 30, 2026

View reviewed changes

jgao54 force-pushed the offload-restore-sensitive-data branch from 478c3f2 to 11f7956 Compare June 30, 2026 06:41

jgao54 force-pushed the offload-restore-sensitive-data branch 2 times, most recently from 7f06742 to 88636d6 Compare June 30, 2026 07:11

jgao54 force-pushed the offload-restore-sensitive-data branch from 88636d6 to f45b4ce Compare June 30, 2026 07:40

ilidemi approved these changes Jul 2, 2026

View reviewed changes

treat string partition ranges as sensitive data

85baeef

jgao54 added 2 commits July 1, 2026 17:24

bug fix + batch insert

b018232

jgao54 force-pushed the offload-restore-sensitive-data branch from f45b4ce to 9bab62a Compare July 2, 2026 03:27

review

38ff3aa

jgao54 merged commit 4ee56fc into main Jul 2, 2026
24 of 25 checks passed

jgao54 deleted the offload-restore-sensitive-data branch July 2, 2026 04:38

	partition = proto.Clone(partition).(*protos.QRepPartition)
	partition = proto.CloneOf(partition)

Uh oh!

Conversation

jgao54 commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ 2 Tests Failed:

Uh oh!

github-actions Bot commented Jun 26, 2026

❌ Test Failure

Uh oh!

github-actions Bot commented Jun 26, 2026

❌ Test Failure

Uh oh!

github-actions Bot commented Jun 26, 2026

❌ Test Failure

Uh oh!

github-actions Bot commented Jun 26, 2026

❌ Test Failure

Uh oh!

claude Bot commented Jun 26, 2026

Code Review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 30, 2026

❌ Test Failure

Uh oh!

claude Bot commented Jun 30, 2026

Code review

Uh oh!

github-actions Bot commented Jun 30, 2026

❌ Test Failure

Uh oh!

claude Bot commented Jun 30, 2026

Uh oh!

github-actions Bot commented Jun 30, 2026

❌ Test Failure

Uh oh!

github-actions Bot commented Jun 30, 2026

❌ Test Failure

Uh oh!

jgao54 commented Jun 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 2, 2026

🔄 Flaky Test Detected

Uh oh!

github-actions Bot commented Jul 2, 2026

🔄 Flaky Test Detected

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jgao54 commented Jun 26, 2026 •

edited

Loading

codecov Bot commented Jun 26, 2026 •

edited

Loading